Hey, I noticed you changed src/languages/en.ts in a PR from a fork. For security reasons, translations are not generated automatically for PRs from forks.

If you want to automatically generate translations for other locales, an Expensify employee will have to:

1. Look at the code and make sure there are no malicious changes.
2. Run the Generate static translations GitHub workflow. If you have write access and the K2 extension, you can simply click: [this button]

Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running:

npx ts-node ./scripts/generateTranslations.ts --help

Typically, you'd want to translate only what you changed by running npx ts-node ./scripts/generateTranslations.ts --compare-ref main

Codecov Report

❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.

@MelvinBot
Review this PR

@codex
Review this PR

Review

Overall this is a well-structured feature — the shared logic is cleanly extracted into useChangeBankAccount, the selector/util changes are focused, and the hook + selector both got unit tests. The bot-flagged items (duplication, the wallet no-op row, VBBA refresh) already look addressed.

My concerns are concentrated in one place: ReimbursementAccountPage, where the "change bank account" flow reuses the same shared reimbursementAccount Onyx data as the connected-account screen and then clobbers it via prepareNewBankAccountSetup. To cope, the page now carries a web of new refs, derived state, and effects — and that is exactly the code with the least test coverage (Codecov confirms the drop there).

Main concerns

1. Fragile route-string equality guard — ReimbursementAccountPage.tsx:242 gates fetchData on backTo === ROUTES.BANK_ACCOUNT_CONNECT_EXISTING_BUSINESS_BANK_ACCOUNT.getRoute(policyIDParam). This compares a fully-serialized route string, so it silently stops matching the moment backTo carries any extra param (its own backTo, a source, trailing slash, etc.). Matching on the route/screen name rather than the exact string would be far more robust.

2. Two overlapping sources of truth for "is changing" — there's the route param isChangingBankAccount and the Onyx flag IS_CHANGING_TO_NEW_BANK_ACCOUNT (isChangingBusinessBankAccount). They model nearly the same intent but gate different branches (display gate L595/L627 vs fetch gate L304). This split is easy to get out of sync later. Worth a comment explaining why both are needed, or deriving one from the other.

3. Several fetchData() triggers reacting to the same signals — there's now a render-phase setIsSettingBA derived from the loading transition (L131-134), an effect that refetches on that same isLoadingWorkspaceReimbursement transition (L271), a focus-based "data got clobbered" refetch (L288), and the original consume-route-intent effect now guarded by isChangingBusinessBankAccount || isLoadingWorkspaceReimbursement (L304). The code comments acknowledge the races and add guards, but correctness here rests entirely on the ordering in which the various Onyx flags settle. This is the part most likely to regress. Please double-check the fast offline→online and rapid back-navigation cases manually.

4. Test gap on the risky code — the added tests cover the hook and selector (pure logic that was already low-risk). None cover the ReimbursementAccountPage state machine, which is where the actual complexity/risk lives. Given how race-sensitive this is, a component/integration test exercising the connected → change → (pick existing / new) → return-to-connected transitions would materially de-risk the change.

Minor / nits

• The Policy.ts additionalData: {policyID} writes are correct with Onyx.METHOD.MERGE (verified), and the NullishDeep<BankAccountList> type change is the right call to allow policyID: null. 👍
• The connectedBankAccount lookup in ConnectExistingBusinessBankAccountPage.tsx leans on additionalData?.policyID (an optional field) for partially-set-up accounts. It falls back to policy.achAccount.bankAccountID for fully-set-up ones, so it's fine, but it does add coupling to that field being populated — worth keeping in mind.
• mapCurrencyToCountry returning Country | '' instead of string is a nice tightening. 👍

None of the above are blockers on their own — the feature logic looks sound. The ask is mainly (a) harden the string-equality guard, (b) a short comment reconciling the two "is changing" flags, and (c) ideally one integration test over the page's transitions given the concentrated race risk.

_{🤖 Generated by MelvinBot — I reviewed the diff and verified specifics against the codebase, but I did not run the app. Treat as a second opinion, not a substitute for manual QA of the flows above.}

Checking all of the feedback, maybe I'll try to simplify the logic! Also, I haven't solved the second reported issue yet

UPD: still trying to correct the second issue.
There is one issue on the new logic side, but it also looks like there is an existing issue in the same flow. Ex: when I login from account B I see one payment account, if I clear cache and reset data I see another account:

I'm figuring out what goes wrong

I've fixed this: #94019 (comment)
But there is separate existing issue (I've prepared recordings from staging):

1. Login to account A and invite account B to workspace as admin.
2. As user A link bank account 1 to workspace -> workflows
3. Login to account B and set another BA account in the workspace -> workflows
4. Get back to user A.
   On the workspace -> workflows you see user B BA set, but if you click on it you see another BA on all set page.

Video:

This happens cause policy has one account set, and user's reimbursementAccount has another one:

cc @joekaufmanexpensify

Hmm, interesting. In your example, if account B changes the Workspace Bank account to Bank account 2 and account A later goes back to the workflows page and taps the bank account row. Wouldn't they then see the option to link a different bank account again?

It's basically the inverse scenario of what Account B originally did to change the Workspace Bank account.

Hmm, interesting. In your example, if account B changes the Workspace Bank account to Bank account 2 and account A later goes back to the workflows page and taps the bank account row. Wouldn't they then see the option to link a different bank account again?

It's basically the inverse scenario of what Account B originally did to change the Workspace Bank account.

Yes, I understand it, but on the video, I provide an example from staging to point out the inconsistency issue. Basically, after user B selects BA 2, it displays for user A on workflows -> payments, but if user A tap on it -> he sees BA1.
The inconsistency is due to policy.achAccount returns one account, and reimbursementAccount.achAccount another one.
See, workflows->payments has 0000 account, but all set displays 1111:

Is it expected that returned from BE data has different accounts in policy.achAccount and reimbursementAccount.achAccount. If so, the All set page then should show BA provided in policy.achAccount?

cc @robertjchen wdyt about this one?

I agree. Seems odd to me. Discussing more here.

Is it expected that returned from BE data has different accounts in policy.achAccount and reimbursementAccount.achAccount. If so, the All set page then should show BA provided in policy.achAccount?

Yes, it's expected that it's different. From my understanding:

• policy.achAccount = the workspace's real, shared payment account (whatever the last admin set)
• reimbursementAccount.achAccount = a per-user view. The backend re-maps the policy account to a copy the viewing user personally owns (because reimbursement runs under the reimburser's own bank account ID). A doesn't own B's account, so A falls back to their own.

So in this case, the display here should use policy.achAccount 🤔

It was agreed to fix this existing issue in a separate PR.
@ZhenjaHorbach, can you please continue the review of this one?

Will recheck soon!

Changes look good!

🚧 robertjchen has triggered a test Expensify/App build. You can view the workflow run here.

🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
Built from App PR #94019.

Android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.us-east-1.amazonaws.com/rock-artifacts/ad-hoc/rock-android-Adhoc-44aa54b1b4a-24d465b-55e74f9fa2bd3cfdd03d8e07bc4df1d577bd757d/index.html	https://ad-hoc-expensify-cash.s3.us-east-1.amazonaws.com/rock-artifacts/ad-hoc/rock-ios-device-AdHoc-44aa54b-24d465b-f5139d4457835fef5fa5ee2d52109726d3027e12/index.html

Web 🕸️
https://94019.pr-testing.expensify.com

👀 View the workflow run that generated this build 👀

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.